Worker Progress Refactoring - #46

/opencode /review pr

Here's my review of the PR:

Summary

Clean refactoring — migrates all Pair<Int, Int>? progress tracking to the Progress data class introduced in #45. Good structured concurrency improvements and dead code removal.

What's Good

• Consistent migration: All 12 files correctly move from ad-hoc Pair<Int, Int>? + workDataOf(KEY_PROGRESS ...) to Progress + toWorkData() / fromWorkData() / fromWorkInfo().
• Structured concurrency: Replacing standalone CoroutineScope(Dispatchers.Default + SupervisorJob()) with coroutineScope { launch { ... } } — proper cancellation propagation, no scope leaks.
• Cooperative cancellation: ensureActive() in the R30UpdateJob loop and CancellationException re-throwing in both jobs are good.
• Progress throttling: sample(100.milliseconds) in ImageHashesDatabaseBuilderJob prevents excessive setProgress() calls to WorkManager.
• Atomic updates: Using progressFlow.update { ... } instead of .value = for read-modify-write in both jobs and ImageHashesDatabaseBuilder.
• Better naming: matchesR30Condition → triggersConditionalWrite, minPlayRatingItem → minByPlayRating, updateR30ListByDate → updateR30ListByDirectWrite, updateR30ListByPotential → updateR30ListByConditionalWrite.
• Dead code removal: R30EntryRepository.updating / updateProgress StateFlows were unused — good riddance.
• Safety: increment() now caps at total via coerceAtMost(total).

Issues

1. Log noise on every normal run (both jobs)

parseRunMode() uses data.getInt(DATA_RUN_MODE, -1) whereas the old code used NORMAL.value (0) as default. Now every normal invocation hits fromInt(-1) → null → logs a warning:

"Invalid RunMode -1, falling back to NORMAL"

This adds unnecessary noise to production logs. Fix by keeping NORMAL.value as the default, or by only logging when the input is actually an unrecognized value (not the default):

// Option A: use NORMAL.value as default (silent fallback)
val runModeInput = inputData.getInt(DATA_RUN_MODE, RunMode.NORMAL.value)
// Option B: don't log when it's just the default sentinel
if (result == null && runModeInput != -1) {
    logger.w { "Invalid RunMode $runModeInput, falling back to ${RunMode.NORMAL}" }
}

2. Magic sentinel in fromWorkData

-75 as a sentinel value in ProgressWorkManager.kt:19-20 is undocumented and arbitrary. If progress values are always non-negative, something like Int.MIN_VALUE would make the intent clearer, but consider a simpler approach:

fun Progress.Companion.fromWorkData(data: Data): Progress? {
    if (!data.keyValueMap.containsKey(KEY_CURRENT) && !data.keyValueMap.containsKey(KEY_TOTAL)) {
        return null
    }
    return Progress(
        current = data.getInt(KEY_CURRENT, 0),
        total = data.getInt(KEY_TOTAL, -1),
    )
}

This avoids sentinel values entirely by checking key presence.

Minor Suggestions

• parseRunMode duplication: Both OcrQueueProcessingJob and R30UpdateJob have identical implementations. Could extract to a shared helper or make RunMode.fromInt include the fallback + logging logic once.

• Comment reference: The doc comment in R30UpdateJob.kt:138 says "Wrapper of [updateR30ListByDirectWrite] and [updateR30ListByConditionalWrite]…" but the methods were renamed. The doc should accurately reference the new method names (actually it does in the new code — this is a non-issue, the diff shows it was updated correctly).

Verdict

Approve after fixing issue #1. The rest is solid.

opencode session  |  github run

/opencode Follow up the new commits

Three follow-up commits address the review's two main issues:

Issue #1 — Log noise (parseRunMode default value): Partially fixed. R30UpdateJob now uses 0 (NORMAL) as default (R30UpdateJob.kt:84), but OcrQueueProcessingJob still uses -1 (OcrQueueProcessingJob.kt:79), so it'll still log a warning on every normal run:

val runModeInput = inputData.getInt(DATA_RUN_MODE, -1)  // should be 0 like R30UpdateJob

Issue #2 — Magic sentinel -75: Fixed. ProgressWorkManager.kt:17-22 now checks keyValueMap.isEmpty() + key presence instead of a magic number, and falls back to Progress.INDETERMINATE values as defaults.

One remaining nit: OcrQueueProcessingJob.kt:79 should change -1 to 0 (or RunMode.NORMAL.value) to match the fix in R30UpdateJob. Otherwise slapping 0c8e14d on just one job while leaving the other untouched seems like an oversight.

opencode session  |  github run